perf(repsel): element-shape versioned loop clone — the first consumer of the element-shape invariant (#7480 / #5093) - #7612
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds guarded element-shape loop cloning for eligible numeric ChangesElement-shape loop cloning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant lower_for
participant element_shape_loop
participant element_shape_guard
participant js_array_ensure_element_shape
participant property_get_helpers
lower_for->>element_shape_loop: match and lower eligible loop
element_shape_loop->>element_shape_guard: emit preheader checks
element_shape_guard->>js_array_ensure_element_shape: validate array element shape
js_array_ensure_element_shape-->>element_shape_guard: return shape class
element_shape_loop->>property_get_helpers: lower arr[i].field
property_get_helpers->>element_shape_guard: emit residual checks and raw field load
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6b99820 to
43e5ae7
Compare
The first consumer of the per-array homogeneous element-shape invariant (#7496, matrix #7608), which landed with no consumer on purpose. `for (let j = 0; j < n; j++) sum += keep[j].v` gets a specialized clone behind a preheader guard on "this array holds the element-shape invariant at class C". The element read becomes a bare gep+load off a cached elements base and the field read a bare raw-f64 slot load; the generic body survives unchanged as the cold arm. Measured 41ms -> 13ms (3.15x), now at parity with node, at +0 bytes on a program with no qualifying loop. Revocation mechanism: restrict-the-body, enforced twice — by shape in the matcher (a single store-free `acc = <pure numeric>` statement) and by construction in the lowering, which scans every emitted block of the fast clone for a GC-unsafe call and branches unconditionally to the slow clone if one survived. Call-freeness is exactly the right property: every way to revoke the invariant (element store, length change, delete, defineProperty, prototype surgery) is a runtime call, and so is every allocation that could move the array. Failure mode: conservative, never unsound. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/perry-codegen/src/expr/element_shape_guard.rs (1)
248-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the
field_countandkeys_arrayObjectHeader offsets, or assert them inconstants_match_the_runtime.
object_header_size_bytescomes fromcrate::target_layout, and the module drift-tests every mirrored runtime mask constant. The+12and+16ObjectHeader field offsets are the same target-dependent layout fact, but they are inline literals. IfObjectHeaderchanges, these reads can target the wrong bytes.Reuse derived offset constants, or add
ObjectHeader::field_countandObjectHeader::keys_arraychecks toconstants_match_the_runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/expr/element_shape_guard.rs` around lines 248 - 254, The ObjectHeader reads in the guard use unvalidated inline offsets. Replace the `12` and `16` literals in the `field_count` and `keys_array` GEPs with target-layout-derived offset constants, or extend `constants_match_the_runtime` with checks for `ObjectHeader::field_count` and `ObjectHeader::keys_array` so these offsets remain synchronized with the runtime.crates/perry-codegen/src/stmt/element_shape_loop_tests.rs (1)
272-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe store test declines on statement count, not on the store.
The body here has two statements. The matcher requires exactly one
Stmt::Expr(Expr::LocalSet(..))body statement, so it declines at the slice pattern and never inspects the store. The test name and the doc comment claim store detection is the reason.The assertion is still correct, but it does not pin the store rule. Consider adding a case whose body is a single statement that writes, for example
Stmt::Expr(Expr::IndexSet { .. })alone, so a future widening of the body shape to multiple statements still fails here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/stmt/element_shape_loop_tests.rs` around lines 272 - 293, Add a separate test for the element-shape versioned loop using a single-statement body containing only Expr::IndexSet, so rejection is specifically exercised by store detection rather than statement-count matching. Keep the existing assertion that no CLONE_LABELS are emitted and retain the current multi-statement case only if it covers a distinct behavior.crates/perry-codegen/src/stmt/element_shape_loop.rs (1)
553-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord that the deref block is outside the call-free scan.
fast_scan_startis read afteremit_element_shape_loop_preheader_checkreturns, soderef_idxsits below the scanned range and is never checked. The SAFETY comment at Line 435 states the call-free window starts at the post-guard re-derivation, which includes that block.The code is safe today because the helper derives
elements_baseas the last pointer computation inderef_idxand emits only loads afterwards. A future edit to that helper could add a call after the derivation and no scan would catch it. Consider scanningderef_idxas well, or stating the exclusion at this site.🛡️ Proposed scan widening
- let fast_clone_call_free = !ctx.func.blocks()[fast_pre_idx].contains_gc_unsafe_call() + let fast_clone_call_free = !ctx.func.blocks()[deref_idx].contains_gc_unsafe_call() + && !ctx.func.blocks()[fast_pre_idx].contains_gc_unsafe_call() && (fast_scan_start..fast_scan_end) .all(|idx| !ctx.func.blocks()[idx].contains_gc_unsafe_call());Note: the guard call itself lives in
query_idx, notderef_idx, so this does not disable the clone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/stmt/element_shape_loop.rs` around lines 553 - 560, Include the dereference block identified by deref_idx in the fast_clone_call_free verification, since the call-free window begins there but the current range starts at fast_scan_start. Preserve the existing fast_pre_idx and subsequent-block checks while widening the scan to cover deref_idx, or explicitly document and enforce its exclusion if that is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/expr/element_shape_guard.rs`:
- Around line 280-335: Update constants_match_the_runtime to derive forwarded,
has_descriptors, typed_intact, and the expected object type from the
corresponding runtime constants rather than local literals. Expose
GC_FLAG_FORWARDED, OBJ_FLAG_HAS_DESCRIPTORS, GC_OBJ_TYPED_LAYOUT_INTACT, and
GC_TYPE_OBJECT through perry-codegen’s public API, then use those exported
symbols when constructing mask and expect so runtime changes are detected.
---
Nitpick comments:
In `@crates/perry-codegen/src/expr/element_shape_guard.rs`:
- Around line 248-254: The ObjectHeader reads in the guard use unvalidated
inline offsets. Replace the `12` and `16` literals in the `field_count` and
`keys_array` GEPs with target-layout-derived offset constants, or extend
`constants_match_the_runtime` with checks for `ObjectHeader::field_count` and
`ObjectHeader::keys_array` so these offsets remain synchronized with the
runtime.
In `@crates/perry-codegen/src/stmt/element_shape_loop_tests.rs`:
- Around line 272-293: Add a separate test for the element-shape versioned loop
using a single-statement body containing only Expr::IndexSet, so rejection is
specifically exercised by store detection rather than statement-count matching.
Keep the existing assertion that no CLONE_LABELS are emitted and retain the
current multi-statement case only if it covers a distinct behavior.
In `@crates/perry-codegen/src/stmt/element_shape_loop.rs`:
- Around line 553-560: Include the dereference block identified by deref_idx in
the fast_clone_call_free verification, since the call-free window begins there
but the current range starts at fast_scan_start. Preserve the existing
fast_pre_idx and subsequent-block checks while widening the scan to cover
deref_idx, or explicitly document and enforce its exclusion if that is required.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1992faae-beb5-4b8c-a27e-453adbc392b2
📒 Files selected for processing (15)
changelog.d/7612-element-shape-loop-clone.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/expr/element_shape_guard.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_get/helpers.rscrates/perry-codegen/src/runtime_decls/arrays.rscrates/perry-codegen/src/stmt/element_shape_loop.rscrates/perry-codegen/src/stmt/element_shape_loop_tests.rscrates/perry-codegen/src/stmt/loops.rscrates/perry-codegen/src/stmt/mod.rscrates/perry-runtime/src/array/element_shape.rstest-files/test_gap_repsel_element_shape_loop_clone.ts
| #[test] | ||
| fn constants_match_the_runtime() { | ||
| assert_eq!(POINTER_TAG_HI16, (0x7FFDu64).to_string()); | ||
| assert_eq!(HANDLE_BAND_TOP, (0x0F_FFFFu64).to_string()); | ||
| assert_eq!(GC_TYPE_ARRAY, "1"); | ||
|
|
||
| // Reconstruct the header mask from the individual runtime constants, | ||
| // positioned by their byte offsets inside the i32 at `obj - 8`. | ||
| let obj_type_mask = 0x0000_00FFu32; | ||
| let forwarded = u32::from(0x80u8) << 8; // GC_FLAG_FORWARDED @ -7 | ||
| let has_descriptors = 0x0800u32 << 16; // OBJ_FLAG_HAS_DESCRIPTORS @ -6 | ||
| let typed_intact = 0x1000u32 << 16; // GC_OBJ_TYPED_LAYOUT_INTACT @ -6 | ||
| let mask = obj_type_mask | forwarded | has_descriptors | typed_intact; | ||
| let expect = u32::from(2u8) /* GC_TYPE_OBJECT */ | typed_intact; | ||
|
|
||
| assert_eq!(ELEM_HEADER_MASK, mask.to_string(), "header mask drifted"); | ||
| assert_eq!( | ||
| ELEM_HEADER_EXPECT, | ||
| expect.to_string(), | ||
| "header expectation drifted" | ||
| ); | ||
|
|
||
| // Sabotage direction: the mask must actually reject each fact. | ||
| let good = expect; | ||
| assert_eq!(good & mask, expect); | ||
| assert_ne!((good | forwarded) & mask, expect, "forwarded not rejected"); | ||
| assert_ne!( | ||
| (good | has_descriptors) & mask, | ||
| expect, | ||
| "descriptors not rejected" | ||
| ); | ||
| assert_ne!( | ||
| (good & !typed_intact) & mask, | ||
| expect, | ||
| "typed-layout downgrade not rejected" | ||
| ); | ||
| assert_ne!((good ^ 1) & mask, expect, "wrong obj_type not rejected"); | ||
| } | ||
|
|
||
| /// The mask reads three adjacent header bytes as one little-endian i32. | ||
| /// Perry emits for aarch64/x86_64 only; assert the assumption explicitly | ||
| /// so a future big-endian target trips here rather than in a field load. | ||
| #[test] | ||
| fn header_word_assumes_little_endian_targets() { | ||
| for triple in [ | ||
| "aarch64-apple-darwin", | ||
| "x86_64-unknown-linux-gnu", | ||
| "aarch64-linux-android", | ||
| "x86_64-pc-windows-msvc", | ||
| ] { | ||
| assert!( | ||
| !triple.starts_with("s390") && !triple.starts_with("powerpc64-"), | ||
| "{triple} is big-endian; ELEM_HEADER_MASK byte positions are LE-only" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Are the runtime header constants public and reachable from perry-codegen?
rg -nP -C2 '\b(GC_FLAG_FORWARDED|OBJ_FLAG_HAS_DESCRIPTORS|GC_OBJ_TYPED_LAYOUT_INTACT|GC_TYPE_OBJECT|GC_TYPE_ARRAY)\b\s*[:=]' crates/perry-runtime/src
# Does perry-codegen already depend on perry-runtime?
fd -t f 'Cargo.toml' crates/perry-codegen --exec rg -n -C3 'perry-runtime|\[dependencies\]' {}
# Is there an authoritative supported-target-triple list to assert against?
rg -nP -C3 'target_triple|supported_targets|is_macos_triple' crates/perry-codegen/src/target_layout.rsRepository: PerryTS/perry
Length of output: 26750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== element_shape_guard constants and tests =="
sed -n '1,140p;240,340p' crates/perry-codegen/src/expr/element_shape_guard.rs
echo "== perry-codegen dependency and feature exports =="
cat crates/perry-codegen/Cargo.toml
printf '\n-- perry-api-manifest exports/publish related lines --\n'
sed -n '1,120p' crates/perry-api-manifest/Cargo.toml 2>/dev/null || true
echo "== constants declaration context =="
sed -n '1,90p' crates/perry-runtime/src/gc/layout.rs
sed -n '12,24p' crates/perry-runtime/src/gc/types.rs
sed -n '910,920p' crates/perry-runtime/src/gc/types.rs
sed -n '978,984p' crates/perry-runtime/src/gc/types.rs
echo "== deterministic local-test failure model =="
python3 - <<'PY'
import re
from pathlib import Path
text = Path('crates/perry-codegen/src/expr/element_shape_guard.rs').read_text()
m = re.search(r'fn constants_match_the_runtime\(\)[\s\S]*?\n \}\n?(\n \/\*|\n #\[test\])', text)
print("FUNCTION_HAS_CONST_TEST:", m is not None)
if m:
body = m.group(0)
# local literals used only within the test: check if runtime names appear
literals_used = bool(re.search(r'\b(0x80|0x0800|0x1000|GC_TYPE_OBJECT|2)\s*=', body))
print("FUNCTION_USES_LOCAL_LITERAL_MASK_PARTS:", literals_used)
print("FUNCTION_CONTAINS_RUNTIME_CONST_NAME_FLAGS:", 'GC_FLAG_FORWARDED' in body)
print("FUNCTION_CONTAINS_RUNTIME_CONST_NAME_DESRIPTORS:", 'OBJ_FLAG_HAS_DESCRIPTORS' in body)
print("FUNCTION_CONTAINS_RUNTIME_CONST_NAME_INTACT:", 'GC_OBJ_TYPED_LAYOUT_INTACT' in body)
print("FUNCTION_CONTAINS_RUNTIME_CONST_NAME_ARRAY_TYPE:", 'GC_TYPE_ARRAY' in body)
PY
echo "== supported triple references =="
rg -n -C 2 'aarch64-apple-darwin|x86_64-unknown-linux-gnu|target_endian|little_endian|supported|triple' crates/perry-codegen/src crates/perry-api-manifest/src 2>/dev/null | head -200Repository: PerryTS/perry
Length of output: 36311
Compare the header constants to the runtime source.
constants_match_the_runtime currently builds mask from local literals (0x80, 0x0800, 0x1000) and expect from 2. If GC_FLAG_FORWARDED, OBJ_FLAG_HAS_DESCRIPTORS, GC_OBJ_TYPED_LAYOUT_INTACT, or GC_TYPE_OBJECT changes at runtime, this test still passes while ELEM_HEADER_MASK and ELEM_HEADER_EXPECT stay wrong. Add the runtime constants to perry-codegen’s public API and compute the constants from those values instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/expr/element_shape_guard.rs` around lines 280 - 335,
Update constants_match_the_runtime to derive forwarded, has_descriptors,
typed_intact, and the expected object type from the corresponding runtime
constants rather than local literals. Expose GC_FLAG_FORWARDED,
OBJ_FLAG_HAS_DESCRIPTORS, GC_OBJ_TYPED_LAYOUT_INTACT, and GC_TYPE_OBJECT through
perry-codegen’s public API, then use those exported symbols when constructing
mask and expect so runtime changes are detected.
43e5ae7 to
1dd3bd8
Compare
Audit before merge — verified, merged as v0.5.1350The guard is verified in depth, and my two sabotages mapped its structure:
Together: each layer is individually load-bearing for its own case, and no One honest miss in my audit worth recording: my hand-rolled perf probe got Root-dominance re-run (codegen change): corpus 129/129, The two design decisions that earn the merge:
The 6.2× |
check_test_registration.py red on main since #7612: the gap test was dark. Registered after verifying zeal+protect stability.
1 participant
The consumer half of #7480; continues #5093's versioned-loop line.
#7496 landed the per-array homogeneous element-shape invariant with no
consumer, on purpose. This is the consumer.
for (let j = 0; j < n; j++) sum += keep[j].vnow gets a specialized clonebehind a preheader guard on "this array holds the element-shape invariant at
class
C". Inside the clone the element read is a baregep + loadoff apreheader-cached elements base and the field read is a bare raw-f64 slot load.
The existing generic body survives unchanged as the cold arm.
keep: Node[],sum += keep[j].vkeep: {v,w}[](object literal)Revocation mechanism, and its failure mode
Restrict-the-body, enforced twice — the second enforcement is the
load-bearing one.
acc = <pure numeric>statement over
arr[counter].fieldreads, numeric locals, numeric literalsand pure arithmetic /
Math. No stores, calls, closures,await, orupdates other than the counter's. No catch-all arm.
one of its blocks is scanned for a GC-unsafe call. If any survived, the
deref block branches unconditionally to the slow clone and the fast
blocks are left as unreachable code.
Call-freeness is exactly the right property, because every way to revoke the
invariant is a runtime call — element store (
layout_note_slot→note_element_store), length change (caught by the record's pinnedverified_len),delete(aTAG_HOLEstore through the same funnel),definePropertyon the array, prototype surgery — and so is every allocationthat could move the array. Codegen's inline element store is the one path
that can skip the note, and only when the array is statically proven numeric
and pointer-free, which an element-shape array can never be.
Failure mode: conservative, never unsound. Anything that writes, calls, or
reads a field the analysis cannot type gets no clone and runs exactly as
today. The residual risk is a silent loss of the optimization, which is why
the codegen tests assert the fast blocks appear in the emitted IR and that
the fast clone contains no
callat all.A useful consequence of the same rule, verified in the emitted IR: under
PERRY_GC_MOVING_LOOP_POLLS=1the back-edge safepoint is itself a call, so thescan fails and the deref block emits an unconditional
br label %element_shape.loop.slow.preheader. The clone stands down in exactlythe configuration where a mid-loop collection could move the array — with no
special case for it anywhere in the code.
The guard tests the live header; the brand is explicit
The preheader calls
js_array_ensure_element_shape— #7496's own querysurface, which reads the array's current
GcHeaderbit and record andself-heals when the record went stale. No inline reimplementation, so no drift
(#7501).
Sequencing is load-bearing. The
GC_TYPE_ARRAYbrand test comes first, sothe pointer handed to the runtime is already branded — an
Arraysubclassinstance is a plain
ObjectHeaderwhose fields overlayArrayHeader's(#7573/#7603). The elements base pointer is derived only after the guard
call returns, from a fresh load of the array's rooted slot: the call can
allocate, and an allocation can move the array.
What the invariant does not prove
element_class_of_bitsprovesPOINTER_TAG, a readableGcHeader,GC_TYPE_OBJECT,OBJECT_TYPE_REGULARandclass_id == Cfor every elementin the verified prefix — exactly the predicates the element-read tier and the
front half of the field-read precheck spend per iteration, so the clone drops
them. It proves nothing about the per-object facts a raw-f64 slot load needs
(
keys_arrayidentity —delete elem.fcompacts the packed slots whilepreserving
class_id— plusfield_count, the descriptor flag, and thetyped-layout intact bit). Dropping those would be the miscompile, so the clone
keeps a residual per-element check, collapsed to one 4-byte load of the three
contiguous header bytes plus two more loads and a single side-exit branch.
Emitted fast body: zero calls, one branch, no volatile gate load.
Folding those facts into the invariant is the documented next slice and needs
an invalidation surface for
delete/defineProperty/ typed downgrade that#7496 deliberately did not open. It should land the way #7496 did: invariant
first, matrix second, consumer third.
Sabotage, both directions
js_array_ensure_element_shapecallsmain)Breaking the guard (every shape fact discarded, per-element check never
side-exits) faults at the
subclass:case — #7603'sObjectHeader-read-as-ArrayHeaderreproduced on demand. Breaking the clone selection leaves thegap test byte-identical to node while the census drops to the same zero the
base compiler emits, so the fallback is behaviour-neutral and the census is
measuring the clone rather than something incidental.
Size (#7566's discipline)
Both recorded traps avoided — runtime trip counts so nothing unrolls, arrays
escaping through
console.logso nothing is scalar-replaced. Measured on themodule object file so the runtime archive does not blur it.
Runtime archive +232 B for the single
keepalive-anchorsstatic — exactly one,for the one symbol codegen now emits a call to; the other four
js_array_element_shape_*entry points stay unanchored and dead-strippable.Verification
test-files/test_gap_repsel_element_shape_loop_clone.tscovers the hotshape plus mid-loop store revocation (direct and via a call), revocation
between two entries of the same loop, subclass receiver (
constandplainly-typed parameter), shape-mismatched and heterogeneous arrays, holes /
sparse /
delete, empty array, per-element typed-layout downgrade, deletedfield, own accessor, frozen element, prototype surgery, every length
mutation, and a bound past the array's length.
array/object/class/repsel/new/prop, 71 tests) against a same-sessionmainbuild: verdict setsidentical — 70 PASS / 1 FAIL on both arms, the failure
(
test_gap_prop_plan_cache_invalidation) pre-existing on both. This isperf(codegen): keep the numeric-array specialization when the array is captured (#6369) #6377's gate.
modules), both gated modes:
--moving-only0 violations with 40/40 seededviolations caught;
--unrooted-allocas --moving-only0 violations over7,860 GC-capable allocas. Allowlist empty and stays empty.
cargo test -p perry-runtime --no-fail-fast1880 passed / 0 failed;cargo test -p perry-codegen --lib685 passed / 0 failed (7 new).cargo fmt --all --check, file-size cap, addr-class ratchet, GC store-siteinventory, workspace-architecture policy: all clean.
Scope
Declared element types (
keep: Node[]) only. #7480's own object-literal kernel(
keep: {v,w}[]) is deliberately out:receiver_class_namereturningNonefor an
Object-typed element is also what makes the number-context field-readhelper decline, so a wider matcher would buy only dead fast-clone IR. Reaching
it means teaching
static_type_of/receiver_class_nameto type anObject-typed property read — the #6377 "more type visibility un-gates latentfast paths" change, which needs its own gap-suite A/B. Element classes with a
base class are also declined: an inherited layout is not described by the
packed slot index alone, and a native base (
extends Array) is the#7573/#7603 hazard itself.
https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Summary by CodeRabbit
New Features
Bug Fixes
Tests